Conversation
|
Hello there! We had a user @rolandoam report trouble running ruby_llm on JRuby due to some Zeitwerk incompatibilities. Those issues are now resolved on JRuby master (10.0.x) and I wanted to start getting JRuby into your CI to prevent any future breakage. The changes here get the test suite running and it mostly passes. There's two kinds of failures that remain:
We currently allow these exceptions to be raised directly from the YAML engine, which could be causing this to fail in a different way than you expect. This behavior accounts for six out of the eight failures.
The '[]' method reported here is actually due to |
JRuby's YAML backend, SnakeYAML, has a default codepoint limit to prevent large content from leading to a DOS. The limit needs to be increased here to allow it to propagate through the rest of the test, or it will raise too early and fail. See ruby/psych#647
|
I have fixed the encoding issue in jruby/jruby#8873 and bumped up the Psych codepoint limit in this PR and now ruby_llm is green on JRuby! I'd love to see it green in your CI too... could someone approve the workflow? The encoding fix will be in JRuby 10.0.1.0 so for now I've bumped the CI job to use |
|
Would be awesome to make RubyLLM run on JRuby as well! However, the |
|
Thank you for approving the run! I will fix the issues, but I strongly disagree with the strict alphabetical ordering of dependencies. In this case, In general I believe that related dependencies should be grouped together in a Gemfile rather than alphabetically ordered, which results in related deps being placed far from one another. When you need to inspect dependencies, logical groupings make it much easier to manage. But it's your project and I'll comply! |
|
I agree with you. If with a rubocop disable comment you're able to skip the alphabetical ordering only for the JRuby dependencies I'd prefer that. |
|
@crmne Oops, I already pushed the change. I can reverse it with a comment if you would prefer. |
|
@headius if there's a comment that would keep the alphabetical order enabled for all deps except JRuby that's what I would prefer. Otherwise the current commit works for me. |
|
@crmne There doesn't seem to be a way to exclude just the JRuby dependencies, unfortunately. |
|
Ok. I'll keep the JRuby dependencies in mind when looking at that list. Thank you! |
|
Looks like there's one last thing to fix (with a comment) |
This avoids RuboCop requiring total ordering of dependencies and gets around the "too many lines" cop for the development block.
|
I figured out that commenting the dependencies and sorting them within that smaller group solves both cops. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
- Coverage 89.72% 88.91% -0.81%
==========================================
Files 75 75
Lines 2812 2860 +48
Branches 556 556
==========================================
+ Hits 2523 2543 +20
- Misses 289 317 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What this does
This PR adds JRuby to CI. The initial set of changes include:
sqlite3gem to instead use the combination ofjdbc-sqlite3andactiverecord-jdbcsqlite3-adaptersupported by JRuby on Rails.rails-7.1job on CI since JRuby's sqlite3 AR adapter does not currently support 7.2+.Type of change
Scope check
Quality check
overcommit --installand all hooks passmodels.json,aliases.json)API changes
Related issues
None.